Skip to content

Support reverse page ordering in sort pushdown phase 1#19848

Open
GaneshPatil7517 wants to merge 7 commits intoapache:mainfrom
GaneshPatil7517:support-reverse-page-sort-pushdown
Open

Support reverse page ordering in sort pushdown phase 1#19848
GaneshPatil7517 wants to merge 7 commits intoapache:mainfrom
GaneshPatil7517:support-reverse-page-sort-pushdown

Conversation

@GaneshPatil7517
Copy link
Contributor

@GaneshPatil7517 GaneshPatil7517 commented Jan 16, 2026

Summary

Implements Phase 1 infrastructure for reverse page ordering in Parquet sort pushdown optimization, addressing issue #19486. This foundation establishes the flag infrastructure necessary for future page-level reversal implementation.

What's Changed

  • Added reverse_pages flag to ParquetSource struct with getter/setter methods
  • Added reverse_pages field to ParquetOpener struct via builder pattern
  • Extended try_reverse_output() to set both reverse_row_groups and reverse_pages flags when optimizing descending sorts
  • Wired flag propagation through the existing FileSource → ParquetOpener → ParquetSource call chain
  • Updated display formatting to show reverse_pages when enabled

Architecture

This implementation follows the established pattern of reverse_row_groups:

  • Infrastructure flag is added to both source and opener structs
  • Flag is set via builder pattern for clean API design
  • Propagation through try_reverse_output() ensures coordination with row group reversal

Testing

  • ✅ All 27 existing reverse-related tests pass
  • ✅ Added 4 new comprehensive tests for reverse_pages functionality:
    • test_reverse_pages_default_value - Verifies default is false
    • test_reverse_pages_with_setter - Verifies setter works correctly
    • test_reverse_pages_clone_preserves_value - Ensures cloning preserves state
    • test_reverse_pages_independent_of_reverse_row_groups - Confirms independent flag operation
  • ✅ No regressions
  • ✅ Code quality verified:
    • cargo fmt - properly formatted
    • cargo clippy -D warnings - no warnings

Phase 1 Design Rationale

This Phase 1 implementation establishes infrastructure for future page-level reversal. Actual page reversal implementation is deferred to Phase 2 because:

  • Arrow-rs ParquetRecordBatchStreamBuilder currently lacks public APIs for page-level reversal
  • Materializing all pages in memory for reversal would have significant performance implications
  • Separating infrastructure (Phase 1) from implementation (Phase 2) enables parallel development

Phase 2 can implement actual page reversal once arrow-rs provides necessary page-level APIs or alternative approaches are available.

Files Modified

  • datafusion/datasource-parquet/src/source.rs - Added reverse_pages field and methods
  • datafusion/datasource-parquet/src/opener.rs - Added reverse_pages field to builder
  • Added comprehensive test coverage

Related Issues

Fix #19486

@GaneshPatil7517
Support reverse page ordering in sort pushdown phase 1
a6e4d8f 
- Add reverse_pages flag to ParquetSource and ParquetOpener
- Wire reverse_pages through try_reverse_output() alongside reverse_row_groups
- Extend try_reverse_output() to set both flags when optimizing descending sorts
- Add comprehensive test coverage for reverse_pages functionality
- Align with existing reverse_row_groups infrastructure pattern

Phase 1 establishes the foundation for page-level reverse ordering. Actual
page-level reversal implementation will be completed in Phase 2 when arrow-rs
provides page-level API support.

Tests:
- All 27 reverse-related tests pass
- Added 4 new tests for reverse_pages flag
- Verified backward compatibility

Closes apache#19486
Copilot AI review requested due to automatic review settings January 16, 2026 07:19
@github-actions github-actions bot added the datasource Changes to the datasource crate label Jan 16, 2026
Copilot AI reviewed Jan 16, 2026
View reviewed changes
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR implements Phase 1 infrastructure for reverse page ordering in Parquet sort pushdown optimization. The change adds a reverse_pages flag that follows the same pattern as the existing reverse_row_groups flag, establishing the foundation for future page-level reversal implementation.

Changes:

  • Added reverse_pages boolean field to ParquetSource and ParquetOpener structs with corresponding getter/setter methods
  • Extended try_reverse_output() to set both reverse_row_groups and reverse_pages flags when optimizing descending sorts
  • Added display formatting support to show when reverse_pages is enabled

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File Description
datafusion/datasource-parquet/src/source.rs Added reverse_pages field with methods, updated try_reverse_output() to set the flag, added display formatting, and included comprehensive unit tests
datafusion/datasource-parquet/src/opener.rs Added reverse_pages field to ParquetOpener struct with #[expect(dead_code)] attribute and updated test builder

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@GaneshPatil7517
Merge upstream/main: add preserve_order field alongside reverse_pages
649f953
@GaneshPatil7517
Fix: Add #[expect(dead_code)] to with_reverse_pages builder method
c8010fc 
This attribute suppresses the dead code warning for Phase 1 implementation.
The method will be used in Phase 2 when actual page reversal is implemented.
@GaneshPatil7517 GaneshPatil7517 force-pushed the support-reverse-page-sort-pushdown branch from 5001db0 to c8010fc Compare January 16, 2026 08:25
@GaneshPatil7517
Update snapshots to include reverse_pages=true in sort pushdown tests
b1add9e 
All test snapshots now reflect that reverse_pages flag is set
alongside reverse_row_groups when optimizing descending sorts.
@github-actions github-actions bot added the core Core DataFusion crate label Jan 16, 2026
@GaneshPatil7517
Update SQL logic test expectations to include reverse_pages=true
8d91fd5 
All EXPLAIN plan outputs now reflect that reverse_pages flag is set
alongside reverse_row_groups when optimizing descending sorts.

Updated test files:
- sort_pushdown.slt (18 instances)
- topk.slt (1 instance)
- create_external_table.slt (1 instance)
@github-actions github-actions bot added the sqllogictest SQL Logic Tests (.slt) label Jan 16, 2026
@GaneshPatil7517 GaneshPatil7517 mentioned this pull request Jan 16, 2026
Open
zhuqi-lucas
zhuqi-lucas reviewed Jan 16, 2026
View reviewed changes
Copy link
Contributor

@zhuqi-lucas zhuqi-lucas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we can implement this until now, the arrow-rs and parquet both not support it.

@GaneshPatil7517
Copy link
Contributor Author

GaneshPatil7517 commented Jan 16, 2026

I don't think we can implement this until now, the arrow-rs and parquet both not support it.

Thank you for the feedback @zhuqi-lucas!

You're absolutely right - that's exactly why this is Phase 1 infrastructure only.

This PR does not attempt to implement actual page-level reversal. Instead, it:

  1. Establishes the infrastructure foundation - Adds the reverse_pages flag and wires it through the ParquetSource → ParquetOpener chain, following the existing reverse_row_groups pattern.
  2. Defers implementation to Phase 2 - The PR includes code comments noting that actual page-level reversal will be implemented in Phase 2 once arrow-rs and parquet add the necessary public APIs.
  3. Aligns with DataFusion patterns - Uses the same builder pattern and infrastructure approach as the existing reverse_row_groups feature.
    The goal of Phase 1 is to establish the groundwork so that when arrow-rs and parquet add page-level reversal support, the DataFusion integration will be straightforward.

Would you like me to add more explicit documentation in the code comments clarifying this Phase 1 vs Phase 2 split? I'm happy to update the PR if needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@zhuqi-lucas zhuqi-lucas zhuqi-lucas left review comments

Copilot code review Copilot Copilot left review comments

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

core Core DataFusion crate datasource Changes to the datasource crate sqllogictest SQL Logic Tests (.slt)

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Support reverse page for sort pushdown phase 1

3 participants

@GaneshPatil7517 @zhuqi-lucas